style: implement Overview design #31

scorebreaker · 2021-03-30T09:07:08Z

Implement Overview design.

stackingcats

Starting to look good, thanks!
Some changes required, though.

src/dashboard/overview/OverviewItem.tsx

stackingcats · 2021-04-01T13:20:57Z

src/dashboard/overview/OverviewItem.tsx

-      !status.status.includes("light mode") && status.status !== "Disabled"
-    );
+  const getStatusIcon = (status: Status) => {
+    if (status.status.includes("Ready")) {


isServiceReady() would give a more accurate answer.

stackingcats · 2021-04-01T13:21:16Z

src/dashboard/overview/OverviewItem.tsx

-  })
-);
+const getCardHeaderBackgroundImage = (props: OverviewItemProps) => {
+  if (props.status.status.includes("Ready")) {


isServiceReady() would give a more accurate answer.

stackingcats · 2021-04-01T13:27:15Z

src/dashboard/overview/OverviewItem.tsx

    );
  };

-  const isDownloadLogsEnabled = (status: Status): boolean => {


By moving the Download Logs button inside the expanded view, there is a problem. As the expanded view is only visible when opendexd is up and running (since the information displayed is not available otherwise), the logs should still be available, no matter what the opendexd status is. Also, the logs should be available for any service that has a container, even if there's no additional information. The easiest solution would be to leave the Download Logs button in the main view. Otherwise in some cases the view should be expanded just to see the Download Logs button which is bizarre.

I moved it back to the main view.

stackingcats · 2021-04-01T13:28:55Z

src/dashboard/overview/ServiceDetails.tsx

+                </div>
+              )}
+              <ButtonBase onClick={handleClose}>
+                <img src={expandIcon} alt="expand" />


Instead of the expand icon, there should be the "opposite" icon that indicates the collapse.

stackingcats · 2021-04-01T13:41:33Z

src/common/components/data-display/LabeledRow.tsx

+            <div className={classes.copyContainer}>
+              <ButtonBase onClick={() => copyToClipboard(value)}>
+                <img src={copyIcon} alt="copy" className={classes.copyIcon} />
+                Copy
+              </ButtonBase>
+            </div>


The icon seems a bit off in two ways:

When the ripple effect appears on click, there's no padding / no space around the text and icon;

On the right of the icon, there's too much space. Would be good to give the content column (middle) more space.

stackingcats · 2021-04-01T13:42:46Z

src/common/components/data-display/LabeledRow.tsx

 import { copyToClipboard } from "../../utils/appUtil";
-import IconButton from "../input/buttons/IconButton";
+import { copyIcon } from "../../../common/utils/svgIcons";
+import ButtonBase from "@material-ui/core/ButtonBase";


There are button components in this project. Let's just use them in order to handle consistent style throughout the project.

stackingcats · 2021-04-01T13:45:40Z

src/dashboard/overview/ServiceDetails.tsx

+import Dialog from "@material-ui/core/Dialog";
+import DialogContent from "@material-ui/core/DialogContent";
+import { downloadIcon, expandIcon } from "../../common/utils/svgIcons";
+import ButtonBase from "@material-ui/core/ButtonBase";


Same here regarding the custom button components.

stackingcats · 2021-04-01T13:46:33Z

src/dashboard/overview/ServiceDetails.tsx

+import { formatDateTimeForFilename } from "../../common/utils/dateUtil";
+import api from "../../api";
+import DialogTitle from "@material-ui/core/DialogTitle";
+import Dialog from "@material-ui/core/Dialog";


There's also a Dialog component in this project. Would really make it lot easier to maintain to use this.

I found it very annoying if I want to design the dialog title part then I will have to give like 3 classes as props, is it necessary to have internal Dialog component?

Wouldn't it be an option to give the title as an input instead?

stackingcats · 2021-04-01T13:52:58Z

src/dashboard/overview/Overview.tsx

+      <div>
+        <div className={classes.headingContainer}>
+          <h1 className={classes.heading}>Overview</h1>
+          <Icon


Please let's hide it until it has a functionality. Just some false condition here would do it.
Also, thinking of other views, they also need it so would not make sense to keep it in the Overview component but a PageTitleComponent or so would be meaningful.

Created PageTitle component for it.

…gs back outside Overview item, use collapse icon instead of expand, created PageTitle component to be reusable with all views

BitcoinOG requested a review from stackingcats March 30, 2021 09:39

stackingcats requested changes Apr 1, 2021

View reviewed changes

style: implement Overview design

c22e450

scorebreaker force-pushed the design-overview branch from dc7d8df to 4e4c4bf Compare April 11, 2021 23:43

style: use isServiceReady to determine ready status, move Download Lo…

55df1a8

…gs back outside Overview item, use collapse icon instead of expand, created PageTitle component to be reusable with all views

scorebreaker force-pushed the design-overview branch from 4e4c4bf to 55df1a8 Compare April 11, 2021 23:49

style: implement Overview design #31

Are you sure you want to change the base?

style: implement Overview design #31

Uh oh!

Conversation

scorebreaker commented Mar 30, 2021

Uh oh!

stackingcats left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!